Skip to content

Modularize out most submodules #2287

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
jerryz123 merged 2 commits into from
Sep 8, 2025
Merged

Modularize out most submodules #2287

jerryz123 merged 2 commits into from
Sep 8, 2025

Conversation

@jerryz123
Copy link
Contributor

@jerryz123 jerryz123 commented Aug 31, 2025

Related PRs / Issues:

Type of change:

  • Bug fix
  • New feature
  • Other enhancement

Impact:

  • RTL change
  • Software change (RISC-V software)
  • Build system change
  • Other

Contributor Checklist:

  • Did you set main as the base branch?
  • Is this PR's title suitable for inclusion in the changelog and have you added a changelog:<topic> label?
  • Did you state the type-of-change/impact?
  • Did you delete any extraneous prints/debugging code?
  • Did you mark the PR with a changelog: label?
  • (If applicable) Did you update the conda .conda-lock.yml file if you updated the conda requirements file?
  • (If applicable) Did you add documentation for the feature?
  • (If applicable) Did you add a test demonstrating the PR?
  • (If applicable) Did you mark the PR as Please Backport?

CI Help:
Add the following labels to modify the CI for a set of features.
Generally, a label added only affect subsequent changes to the PR (i.e. new commits, force pushing, closing/reopening).
See ci:* for full list of labels:

  • ci:fpga-deploy - Run FPGA-based E2E testing
  • ci:local-fpga-buildbitstream-deploy - Build local FPGA bitstreams for platforms that are released
  • ci:disable - Disable CI

@jerryz123 jerryz123 changed the base branch from modular to main August 31, 2025 21:44
@jerryz123 jerryz123 force-pushed the modular-more branch 13 times, most recently from 76580b2 to 4929918 Compare September 1, 2025 18:19
@jerryz123 jerryz123 changed the base branch from main to modular September 1, 2025 18:27
@jerryz123 jerryz123 changed the base branch from modular to main September 1, 2025 21:04
@jerryz123 jerryz123 changed the title Modularize out cores Modularize out most submodules Sep 1, 2025
@iansseijelly
Copy link
Contributor

iansseijelly commented Sep 8, 2025

From Codex:
Summary
Documentation builds now initialize all generator submodules before invoking Sphinx, ensuring that literalincludes from nested repos resolve correctly.
The SBT build gains a withInitCheck helper and an optionalModules list, allowing generator projects to be conditionally compiled only when their submodules are initialized.
common.mk drops explicit submodule checks but still advertises a check-submodule-status target that no longer exists.

Review
The help text in common.mk still references check-submodule-status, yet the target has been removed, which can mislead users navigating available Make targets.
Suggestion: Remove the check-submodule-status line from the HELP_COMMANDS list, and ensure no other references remain.
withInitCheck suggests running init-submodules-no-riscv-tools-nolog.sh, while CI uses init-submodules-no-riscv-tools.sh --full; standardizing on one script name would reduce user confusion.
Suggestion: Update the error message in withInitCheck to reference init-submodules-no-riscv-tools.sh, matching the GitHub workflow. Ensure documentation is consistent.
docs/Generators/fft.rst references code from fft-generator but lacks guidance that this submodule must be initialized, which could cause doc builds to fail on fresh clones.
Suggestion: Add a note near the top explaining that fft-generator must be initialized (e.g., via scripts/init-submodules-no-riscv-tools.sh --full) before using FFTRocketConfig or building the docs.

@iansseijelly
Copy link
Contributor

iansseijelly commented Sep 8, 2025

Also trying this. @codex review

chatgpt-codex-connector[bot]
chatgpt-codex-connector bot reviewed Sep 8, 2025
View reviewed changes
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex Review: Here are some suggestions.

chipyard/generators/firechip/chip/src/main/scala/TargetConfigs.scala

Lines 267 to 299 in e056889

// Gemmini NN accel config, base off chipyard's GemminiRocketConfig
//******************************************************************
class FireSimGemminiRocketConfig extends Config(
new WithDefaultFireSimBridges ++
new WithFireSimConfigTweaks ++
new chipyard.GemminiRocketConfig)
class FireSimLeanGemminiRocketConfig extends Config(
new WithDefaultFireSimBridges ++
new WithFireSimConfigTweaks ++
new chipyard.LeanGemminiRocketConfig)
class FireSimLeanGemminiPrintfRocketConfig extends Config(
new WithDefaultFireSimBridges ++
new WithFireSimConfigTweaks ++
new chipyard.LeanGemminiPrintfRocketConfig)
//**********************************************************************************
// Supernode Configurations, base off chipyard's RocketConfig
//**********************************************************************************
class SupernodeFireSimRocketConfig extends Config(
new WithFireSimHarnessClockBridgeInstantiator ++
new chipyard.harness.WithHomogeneousMultiChip(n=4, new Config(
new freechips.rocketchip.subsystem.WithExtMemSize((1 << 30) * 8L) ++ // 8GB DRAM per node
new FireSimRocketConfig)))
//**********************************************************************************
//* CVA6 Configurations
//*********************************************************************************/
class FireSimCVA6Config extends Config(
new WithDefaultFireSimBridges ++
new WithFireSimConfigTweaks ++
new chipyard.CVA6Config)

[P1] Guard FireSim configs behind optional generator discovery

Chipyard now removes Gemmini and CVA6 mixins from the core repo and only adds their projects when the submodule is initialized, but FireSim still unconditionally declares FireSimGemminiRocketConfig and FireSimCVA6Config in this file. When someone follows the new minimal submodule setup (leaving generators/gemmini or generators/cva6 uninitialized), sbt firechip/compile fails with not found: type chipyard.GemminiRocketConfig/chipyard.CVA6Config because the classes live in those optional repos and are no longer on the classpath. These configs should either live alongside their generator or be conditionally generated only when the corresponding submodule is present so that FireSim can compile without optional accelerators.

Reply with @codex fix comments to fix any unresolved comments.

About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".

@jerryz123
Copy link
Contributor Author

jerryz123 commented Sep 8, 2025

Oh amazing. Ty

@jerryz123 jerryz123 merged commit 90745a2 into main Sep 8, 2025
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chatgpt-codex-connector chatgpt-codex-connector[bot] chatgpt-codex-connector[bot] left review comments

@iansseijelly iansseijelly iansseijelly approved these changes

Assignees

No one assigned

Labels

changelog:added

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jerryz123 @iansseijelly